Skip to content

WIP: Low cardinality span names + callback for WSGI#440

Closed
Skeen wants to merge 2 commits intoopen-telemetry:masterfrom
Skeen:ticket/409_low_cardinality_span_names
Closed

WIP: Low cardinality span names + callback for WSGI#440
Skeen wants to merge 2 commits intoopen-telemetry:masterfrom
Skeen:ticket/409_low_cardinality_span_names

Conversation

@Skeen
Copy link
Copy Markdown
Contributor

@Skeen Skeen commented Feb 21, 2020

This PR fixes #409, and has an ASGI counterpart here: ede28b2

@Skeen Skeen requested a review from a team February 21, 2020 16:57
@Skeen Skeen changed the title Low cardinality span names + callback for WSGI WIP: Low cardinality span names + callback for WSGI Feb 21, 2020
@Skeen
Copy link
Copy Markdown
Contributor Author

Skeen commented Feb 21, 2020

I see the issues with the flask integration, I'll look into it.

"""

def __init__(self, wsgi):
def __init__(self, wsgi, name_callback=None):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just use get_default_span_name as default here:

def function(argument):
    return "prefix_0_" + argument


class Middleware:

    def __init__(self, callback=function):
        self.callback = callback

print(Middleware().callback("argument"))
print(Middleware(lambda x: "prefix_1_" + x).callback("argument"))
prefix_0_argument
prefix_1_argument

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The middleware is initialized only once per process right? Is it worth it adding a dedicated callback to init for this? What if the get_default_span_name() is a method on the middleware class? Users can then just override the class and change the default logic. If we don't want users to go through overriding classes, may be we can provide option constructors in addition? Something like, OpenTelemetryMiddleware.with_span_name_setter(name_setter_func).

I don't think this is a big deal but it feels we'll end up adding more and more args to init to allow customizing some behaviour. Might be worth it prescribing sub-classing instead.

Either that or perhaps the callback could be a generic span callback that gets the whole span and can modify the span in any way, setting the name being one.

@lzchen lzchen closed this in #837 Jun 23, 2020
@Skeen Skeen deleted the ticket/409_low_cardinality_span_names branch June 23, 2020 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

extensions should use low cardinality span names by default.

4 participants